Discard likely duplicate problem notifications via Notification#last_notified_state_per_user #9893

Al2Klimov · 2023-11-07T10:00:20Z

Commits

Whenever a state notification is sent: track user.name=>checkable.state in Notification#last_notified_state_by_user
On Notification#last_notified_state_by_user change: sync that over cluster
Among the other checks like is-acked, matches-filter, etc. in Notification#BeginExecuteNotification(): in case of the same non-reminder problem already notified before drop the eff. dupl. notification
On recovery: clear that state

Test

Tested with https://github.com/Al2Klimov/twintowers and following config in volumes/icinga2/master1/etc/icinga2/zones.d/master/my.conf

 for (i in range(100)) {
    object Host i {
        check_command = "dummy"
        enable_active_checks = false
        #check_interval = "dummy"
        #vars.dummy_state = {{ Math.random() < 0.01 ? 2 : 0 }}
        #vars.dummy_text = "-"
    }
}

 for (i in range(10)) {
apply Service i {
        check_command = "dummy"
        enable_active_checks = false
    assign where true
}
}

object User "me" {
    states = [ OK, Critical, Unknown ]
}

object NotificationCommand "noop" {
    command = [ "true" ]
}

apply Notification "test" to Host {
    command = "noop"
    users = [ "me" ]
    assign where true
}

apply Notification "test" to Service {
    command = "noop"
    users = [ "me" ]
    assign where true
}

👍 Initial (passive) warning filtered out as configured
👍 Initial critical notified as configured
👍 Follow-up warning filtered out as configured
👍 Follow-up critical filtered out due to this change
👍 Happened while one master was down on 10/10 services, so HA works

yhabteab

Apart from the comments below, please also document the new cluster events here.

lib/icinga/notification.ti

lib/icinga/notification.cpp

lib/icinga/clusterevents.cpp

test/icinga-notification.cpp

yhabteab · 2023-11-24T16:04:32Z

test/icinga-notification.cpp

+	Application::GetTP().Start();
+	helper.n->BeginExecuteNotification(NotificationRecovery, nullptr, false, false, "", "");
+	Application::GetTP().Stop();
+	BOOST_CHECK(helper.called == 1u);


It might also be worth adding this test here:

BOOST_CHECK(helper.n->GetLastNotifiedStatePerUser()->Get(helper.u->GetName()) == Empty);

Redundant IMAO. It's only the bridge to (not) sending a notification, the latter is important and tested here.

yhabteab

Can you please also deduplicate the remaining test cases.

test/icinga-notification.cpp

julianbrost · 2023-11-30T09:28:40Z

lib/icinga/clusterevents.cpp

+	notification->GetLastNotifiedStatePerUser()->Set(params->Get("user"), state);
+	Notification::OnLastNotifiedStateOfUserUpdated(notification, params->Get("user"), state, origin);


The attributes and signals are named inconsistently ("per" vs. "of"). For what it's worth, my preference would be unifying towards "per".

Guilty. To my defence: The whole thing contains all states, separately per user. But sometimes I want to update only the value of a particular user.

But wouldn't it make so much more sense if everything that's specifically related to syncing that attribute was also named like that attribute and not just somewhat similar? Actually, this affects OnLastNotifiedStatesCleared as well.

lib/icinga/notification.cpp

test/CMakeLists.txt

test/icinga-notification.cpp

…ions

doc/19-technical-concepts.md

lib/icinga/clusterevents.cpp

doc/19-technical-concepts.md

yhabteab

Warning ⚠️:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 1, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:17:21 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'icinga2!test!notify'.
[2023-12-12 16:17:21 +0100] notice/Notification: Not sending notifications for notification object 'icinga2!test!notify and user 'yhabteab': state 'Warning' does not match state filter: Critical, OK and Unknown.

Critical:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 2, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:18:35 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'icinga2!test!notify'.
[2023-12-12 16:18:35 +0100] information/Notification: Sending 'Problem' notification 'icinga2!test!notify' for user 'yhabteab'
[2023-12-12 16:18:35 +0100] information/Notification: Completed sending 'Problem' notification 'icinga2!test!notify' for checkable 'icinga2!test' and user 'yhabteab' using command 'noop'.

Critical -> Warning -> Critical:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 1, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 2, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:21:48 +0100] notice/Notification: Attempting to send notifications of type 'Problem' for notification object 'icinga2!test!notify'.
[2023-12-12 16:21:48 +0100] notice/Notification: Notification object 'icinga2!test!notify': We already notified user 'yhabteab' for a Critical problem. Likely after that another state change notification was filtered out by config. Not sending duplicate 'Critical' notification.

Recovery -> Critical:

# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 0, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:24:00 +0100] information/Notification: Completed sending 'Recovery' notification 'icinga2!test!notify' for checkable 'icinga2!test' and user 'yhabteab' using command 'noop'.


# curl -k -s -S -i -u root:icinga -H 'Accept: application/json' \
 -X POST 'https://localhost:5666/v1/actions/process-check-result' \
-d '{ "type": "Service", "filter": "service.name==\"test\"", "exit_status": 2, "plugin_output": "PING CRITICAL - Packet loss = 100%", "performance_data": [ "rta=5000.000000ms;3000.000000;5000.000000;0.000000", "pl=100%;80;100;0" ], "pretty": true }'

[2023-12-12 16:24:20 +0100] information/Notification: Completed sending 'Problem' notification 'icinga2!test!notify' for checkable 'icinga2!test' and user 'yhabteab' using command 'noop'.

julianbrost · 2023-12-13T09:44:40Z

test/icinga-notification.cpp

+		n->SetUsersRaw(new Array({"jdoe"}), true);
+		n->SetTypeFilter(typeFilter);
+		n->SetStateFilter(stateFilter);
+		((ConfigObject*)n.get())->OnAllConfigLoaded(); // link Service


What's the purpose of this cast? ConfigObject::OnAllConfigLoaded() is virtual, so shouldn't this just call n->OnAllConfigLoaded() anyways?

And if a cast is actually necessary, we have static_pointer_cast/dynamic_pointer_cast.

Config::OnAllConfigLoaded() is virtual, so shouldn't this just call n->OnAllConfigLoaded() anyways?

I suppose it's because Notification::OnAllConfigLoaded() isn't publicly accessible.

And if a cast is actually necessary, we have static_pointer_cast/dynamic_pointer_cast.

I don't get the point of these. I'm casting to base, not to derived.

Is there any good reason why Notification::OnAllConfigLoaded() shouldn't be public if it implements a public virtual method of a base class? So unless there's a good reason, I'd just change the visibility and avoid strange casts altogether.

Absolutely! IMAO always the overriding method of the most derived class shall be called from external, none of the base methods. Protecting all ex. the base virtual one enforces this. OK, admittedly Notification is the most derived class. But if this changes for Notification (or any other class) in the future, one had to remember these rules and make all methods protected the new derived class overrides. To make some already public methods protected!

Either I'm getting you wrong or what you're describing can't happen.

https://en.cppreference.com/w/cpp/language/virtual#In_detail:

If some member function vf is declared as virtual in a class Base, and some class Derived, which is derived, directly or indirectly, from Base, has a declaration for member function with the same [...]
Then this function in the class Derived is also virtual (whether or not the keyword virtual is used in its declaration) and overrides Base::vf (whether or not the word override is used in its declaration).

So Notification::OnAllConfigLoaded() is virtual automatically and if there was another derived class overriding it, it would still be used.

julianbrost · 2023-12-13T10:02:03Z

lib/icinga/notification.hpp

@@ -92,6 +93,8 @@ class Notification final : public ObjectImpl<Notification>
 	static String NotificationHostStateToString(HostState state);

 	static boost::signals2::signal<void (const Notification::Ptr&, const MessageOrigin::Ptr&)> OnNextNotificationChanged;
+	static boost::signals2::signal<void (const Notification::Ptr&, const String&, uint_fast8_t, const MessageOrigin::Ptr&)> OnLastNotifiedStatePerUserUpdated;
+	static boost::signals2::signal<void (const Notification::Ptr&, const MessageOrigin::Ptr&)> OnLastNotifiedStatesCleared;


I'd also rename this to OnLastNotifiedStatePerUserCleared. Why? Because a simple git grep LastNotifiedStatePerUser will then lead you to all the places that affect this attribute.

…blem notifications

…ied_state_per_user

Al2Klimov added the consider backporting Should be considered for inclusion in a bugfix release label Nov 7, 2023

Al2Klimov added this to the 2.15.0 milestone Nov 7, 2023

Al2Klimov requested a review from julianbrost November 7, 2023 10:00

cla-bot bot added the cla/signed label Nov 7, 2023

icinga-probot bot added area/configuration DSL, parser, compiler, error handling area/notifications Notification events enhancement New feature or request needs-sponsoring Not low on priority but also not scheduled soon without any incentive labels Nov 7, 2023

julianbrost requested a review from yhabteab November 21, 2023 09:30

yhabteab requested changes Nov 22, 2023

View reviewed changes

lib/icinga/notification.ti Outdated Show resolved Hide resolved

lib/icinga/notification.cpp Outdated Show resolved Hide resolved

lib/icinga/clusterevents.cpp Outdated Show resolved Hide resolved

yhabteab removed the needs-sponsoring Not low on priority but also not scheduled soon without any incentive label Nov 22, 2023

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from 40244e6 to 4b51ec9 Compare November 24, 2023 12:41

Al2Klimov requested a review from yhabteab November 24, 2023 12:41

yhabteab requested changes Nov 24, 2023

View reviewed changes

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from 4b51ec9 to 5adefce Compare November 24, 2023 16:22

Al2Klimov requested a review from yhabteab November 24, 2023 16:25

yhabteab requested changes Nov 27, 2023

View reviewed changes

test/icinga-notification.cpp Outdated Show resolved Hide resolved

julianbrost requested changes Nov 30, 2023

View reviewed changes

Notification#BeginExecuteNotification(): track state change notificat…

b25ba7a

…ions

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch 2 times, most recently from 1055f29 to d72906b Compare December 7, 2023 14:18

Al2Klimov requested a review from julianbrost December 8, 2023 10:21

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from d72906b to c1fe30b Compare December 11, 2023 10:31

Al2Klimov requested a review from yhabteab December 11, 2023 16:35

yhabteab requested changes Dec 12, 2023

View reviewed changes

doc/19-technical-concepts.md Outdated Show resolved Hide resolved

lib/icinga/clusterevents.cpp Show resolved Hide resolved

Cluster-sync Notification#last_notified_state_per_user

2cff763

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from c1fe30b to 77a2494 Compare December 12, 2023 14:34

Al2Klimov requested a review from yhabteab December 12, 2023 14:34

yhabteab requested changes Dec 12, 2023

View reviewed changes

doc/19-technical-concepts.md Outdated Show resolved Hide resolved

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from 77a2494 to b3b86a6 Compare December 12, 2023 14:55

Al2Klimov requested a review from yhabteab December 12, 2023 14:55

yhabteab previously approved these changes Dec 12, 2023

View reviewed changes

julianbrost requested changes Dec 13, 2023

View reviewed changes

Al2Klimov dismissed yhabteab’s stale review via ef153bc December 13, 2023 10:30

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from b3b86a6 to ef153bc Compare December 13, 2023 10:30

Al2Klimov requested a review from julianbrost December 13, 2023 10:30

Al2Klimov added 2 commits December 13, 2023 13:21

Notification#BeginExecuteNotification(): discard likely duplicate pro…

44e9c6f

…blem notifications

Notification#BeginExecuteNotification(): on recovery clear last_notif…

97cd05d

…ied_state_per_user

Al2Klimov force-pushed the do-not-re-notify-if-filtered-states-don-t-change-4503 branch from ef153bc to 97cd05d Compare December 13, 2023 12:21

julianbrost changed the title ~~Discard likely duplicate problem notifications via Notification#last_notified_state_by_user~~ Discard likely duplicate problem notifications via Notification#last_notified_state_per_user Dec 13, 2023

julianbrost approved these changes Dec 13, 2023

View reviewed changes

julianbrost requested a review from yhabteab December 13, 2023 13:59

yhabteab approved these changes Dec 13, 2023

View reviewed changes

Al2Klimov merged commit 953eeba into master Dec 13, 2023
25 checks passed

Al2Klimov deleted the do-not-re-notify-if-filtered-states-don-t-change-4503 branch December 13, 2023 15:13

Al2Klimov mentioned this pull request Dec 13, 2023

Discard likely duplicate problem notifications via Notification#last_notified_state_per_user #9932

Merged

yhabteab added the backported Fix was included in a bugfix release label Dec 18, 2023

julianbrost mentioned this pull request Dec 18, 2023

PluginNotificationTask::ScriptFunc(): on Linux truncate output and comment #9887

Merged

Al2Klimov removed the consider backporting Should be considered for inclusion in a bugfix release label May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discard likely duplicate problem notifications via Notification#last_notified_state_per_user #9893

Discard likely duplicate problem notifications via Notification#last_notified_state_per_user #9893

Al2Klimov commented Nov 7, 2023

yhabteab left a comment •

edited

Loading

yhabteab Nov 24, 2023

Al2Klimov Nov 24, 2023

yhabteab left a comment

julianbrost Nov 30, 2023

Al2Klimov Dec 7, 2023

julianbrost Dec 11, 2023

yhabteab left a comment •

edited

Loading

julianbrost Dec 13, 2023 •

edited

Loading

yhabteab Dec 13, 2023

Al2Klimov Dec 13, 2023

julianbrost Dec 13, 2023

Al2Klimov Dec 13, 2023

julianbrost Dec 13, 2023

julianbrost Dec 13, 2023

		notification->GetLastNotifiedStatePerUser()->Set(params->Get("user"), state);
		Notification::OnLastNotifiedStateOfUserUpdated(notification, params->Get("user"), state, origin);

Discard likely duplicate problem notifications via Notification#last_notified_state_per_user #9893

Discard likely duplicate problem notifications via Notification#last_notified_state_per_user #9893

Conversation

Al2Klimov commented Nov 7, 2023

Commits

Test

yhabteab left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhabteab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhabteab left a comment • edited Loading

Choose a reason for hiding this comment

julianbrost Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhabteab left a comment •

edited

Loading

yhabteab left a comment •

edited

Loading

julianbrost Dec 13, 2023 •

edited

Loading